Botorch preset#757
Conversation
a149bb3 to
c3885f6
Compare
c3885f6 to
4f4fd55
Compare
There was a problem hiding this comment.
Pull request overview
Adds a new BOTORCH Gaussian Process preset intended to exactly mirror BoTorch GP behavior (single- and multi-task), alongside supporting kernel-factory refactors and validation improvements.
Changes:
- Introduce
GaussianProcessPreset.BOTORCHand implement BoTorch-aligned kernel / mean / likelihood factories (including custom GPyTorch components for multitask mean + likelihood). - Add parameter-kind classification (
ParameterKind) and automatic parameter-kind validation for kernel factories via a new_KernelFactorybase. - Extend tests (BoTorch parity + kernel-factory validation) and update the Streamlit surrogate-model demo to expose GP presets and multitask/transfer-learning toggles.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
baybe/surrogates/gaussian_process/presets/botorch.py |
Adds BoTorch preset factories for kernel/mean/likelihood. |
baybe/surrogates/gaussian_process/components/_gpytorch.py |
Introduces custom GPyTorch mean + multitask likelihood helper to match BoTorch behavior. |
baybe/surrogates/gaussian_process/components/kernel.py |
Adds _KernelFactory w/ parameter-kind validation; adjusts ICMKernelFactory. |
baybe/surrogates/gaussian_process/presets/{edbo,edbo_smoothed}.py |
Migrates kernel factories to _KernelFactory and updates dimension logic. |
baybe/surrogates/gaussian_process/presets/{core,__init__.py} |
Adds BOTORCH enum entry and re-exports preset factories. |
baybe/parameters/{enum,base.py} |
Adds ParameterKind and exposes Parameter.kind. |
baybe/parameters/selectors.py |
Removes _ParameterSelectorMixin (superseded by _KernelFactory). |
tests/test_gp.py |
Adds parity test asserting BOTORCH preset reproduces BoTorch posterior stats. |
tests/test_kernel_factories.py |
Adds tests for kernel-factory parameter-kind validation. |
streamlit/surrogate_models.py |
Adds GP preset selector + multitask / transfer-learning UI and logic. |
docs/conf.py |
Updates nitpick ignore list for refactor. |
CHANGELOG.md |
Documents new presets and kernel-factory parameter-kind validation. |
Comments suppressed due to low confidence (1)
baybe/surrogates/gaussian_process/presets/edbo.py:85
effective_dimsis computed astrain_x.shape[-1], which includes the task feature column in transfer-learning searchspaces. Since this factory explicitly excludesTaskParameterviaparameter_selector, the priors should likely be based on the non-task dimensionality (i.e. exclude the task feature) to avoid skewing the dimension-dependent prior heuristics. Consider derivingeffective_dimsfrom the selected non-task dimensions instead (e.g., subtract task dims or count comp-rep columns for selected parameters).
effective_dims = train_x.shape[-1]
switching_condition = _contains_encoding(
searchspace.discrete, _EDBO_ENCODINGS
) and (effective_dims >= 50)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| base_idcs = ( | ||
| set(dims) | ||
| if (dims := base_kernel.active_dims.tolist()) is not None | ||
| else None | ||
| ) | ||
| task_idcs = ( | ||
| set(dims) | ||
| if (dims := task_kernel.active_dims.tolist()) is not None | ||
| else None |
There was a problem hiding this comment.
base_kernel.active_dims / task_kernel.active_dims can be None in GPyTorch kernels. Calling .tolist() unconditionally will raise an AttributeError (NoneType has no attribute tolist). Consider guarding with an is not None check (or using getattr(..., None)) before converting to a set, and treat None explicitly in the validation logic.
| base_idcs = ( | |
| set(dims) | |
| if (dims := base_kernel.active_dims.tolist()) is not None | |
| else None | |
| ) | |
| task_idcs = ( | |
| set(dims) | |
| if (dims := task_kernel.active_dims.tolist()) is not None | |
| else None | |
| base_active_dims = base_kernel.active_dims | |
| task_active_dims = task_kernel.active_dims | |
| base_idcs = ( | |
| all_idcs | |
| if base_active_dims is None | |
| else set(base_active_dims.tolist()) | |
| ) | |
| task_idcs = ( | |
| all_idcs | |
| if task_active_dims is None | |
| else set(task_active_dims.tolist()) |
| 0 if st_enable_multitask else 1, | ||
| 20, | ||
| 0 if st_enable_multitask else 5, |
There was a problem hiding this comment.
When Multi-task is enabled, the on-task training-point slider allows 0. In the st_use_separate_gps branch (transfer learning unchecked), the on-task recommender is still run on the on-task subset, which will be empty and will likely raise during GP fitting/recommendation. Consider enforcing a minimum of 1 on-task point when st_use_separate_gps is True, or add an explicit guard that shows a Streamlit error and stops early when the on-task dataset is empty.
| 0 if st_enable_multitask else 1, | |
| 20, | |
| 0 if st_enable_multitask else 5, | |
| 1, | |
| 20, | |
| 5, |
| if base_idcs is not None and (base_idcs > allowed_base_idcs): | ||
| raise ValueError( |
There was a problem hiding this comment.
The subset check for base-kernel active_dims is incorrect: base_idcs > allowed_base_idcs checks for a strict superset, not “not a subset”. This will miss invalid cases (e.g. {0, task_idx}) and potentially flag none. Use a proper subset validation (e.g. not base_idcs <= allowed_base_idcs) and consider a clearer error if active_dims is None (meaning “all dims”).
| if base_idcs is not None and (base_idcs > allowed_base_idcs): | |
| raise ValueError( | |
| if base_idcs is None: | |
| raise ValueError( | |
| "The base kernel's 'active_dims' must be restricted to the non-task " | |
| f"indices {allowed_base_idcs}; got None, which means all dimensions." | |
| ) | |
| if not base_idcs <= allowed_base_idcs: | |
| raise ValueError( |
680d8a1 to
eeaa63d
Compare
e409228 to
eeaa63d
Compare
AVHopp
left a comment
There was a problem hiding this comment.
The streamlit changes should be a separate PR as they have nothing to do with new presets. I do see the value in extending the streamlit but those two things should still not be combined imo.
eeaa63d to
7bb590b
Compare
AVHopp
left a comment
There was a problem hiding this comment.
Two bugs or uncatched errors in the streamlit
| 0 if st_enable_multitask else 5, | ||
| ) | ||
| if st_enable_multitask: | ||
| st_n_training_points_other = st.sidebar.slider( |
There was a problem hiding this comment.
I got a nasty uncatched ValueError here when activating Multi-Task under Domain, setting Number of training points to some value >0, setting Number of off-task training points to 0 and de-activating Transfer Learning under Model. This should be a more meaningful error (unless it demonstrates a real issue, in which case this should be fixed):
There was a problem hiding this comment.
So the question to me is how we want/should go about this in general. The streamlit was never intended as a user-facing application but is for development only. I'm thus very hesitant to apply all sorts of input validation steps to something what is pretty much a moving target.
Scienfitz
left a comment
There was a problem hiding this comment.
sneaky global change to mll is not acceptable
7bb590b to
8712965
Compare
Criterion is a BayBEGPComponent (check is already included)
11ff22d to
b255188
Compare
0eb5cc0 to
7ac4c25
Compare
7ac4c25 to
9b90df2
Compare
9b90df2 to
d8a5a9b
Compare

DevPR, parent is #745
Adds the
BOTORCHpreset for GPs.Important information
this is the BoTorch behaviorseems like a bad idea. The test ensures this explicitly.